Skip to content

Conversation

@JoeKar
Copy link
Collaborator

@JoeKar JoeKar commented Jan 29, 2024

The performance of the current parsing approach can't be improved without changing the whole highlighter code. Due to this the change isn't without any risk, but it's definitely worth the try. Please see the following list, which has been done with the same host and micro -profile. The test has been stopped after complete highlight within 80x24 or aborted due to known very long and inefficient recursion (DNF). Afterwards the top1 has been printed with pprof.

file references before after
1. 1.93s 12.26% 12.26% 1.93s 12.26% unicode/utf8.DecodeRune 10ms 100% 100% 10ms 100% runtime.futex
2. (DNF) 5.41s 14.64% 14.64% 5.41s 14.64% unicode/utf8.DecodeRune 10ms 100% 100% 10ms 100% runtime.writeHeapBits.flush
3. 10ms 20.00% 20.00% 10ms 20.00% github.com/zyedidia/tcell/v2.(*tScreen).SetContent 20ms 40.00% 40.00% 20ms 40.00% github.com/zyedidia/micro/v2/internal/util.CharacterCount
4. 10ms 20.00% 20.00% 10ms 20.00% crypto/md5.block 10ms 25.00% 25.00% 10ms 25.00% gopkg.in/yaml%2ev2.yaml_parser_update_buffer
5. 10ms 50.00% 50.00% 10ms 50.00% gopkg.in/yaml%2ev2.yaml_parser_scan_plain_scalar 10ms 33.33% 33.33% 10ms 33.33% runtime.(*consistentHeapStats).acquire
6. (DNF) 8.79s 27.01% 27.01% 14.53s 44.65% regexp.(*Regexp).tryBacktrack 10ms 20.00% 20.00% 10ms 20.00% github.com/zyedidia/micro/v2/internal/util.DecodeCharacter
  1. tileset_env_test from Micro does not respnoe when edit specific file #3115 (reduced version)
  2. tileset_env_test from Micro does not respnoe when edit specific file #3115
  3. sample.md from Poor performance on string highlighting #2839
  4. sample.md from Poor performance on string highlighting #2839 (with inserted <script>)
  5. Firefox's new tab page (reduced version)
  6. Firefox's new tab page

My available test files created the same or even more complex highlighting (e.g. pattern highlight within regions in HTMLs) results.
Most probably the logic isn't in a perfect shape yet, but definitely feasible as proof of concept thought.

Please help to test and improving it with a review.
It took a lot of days to get this far and would be a shame when we didn't get this upstream in any form. 😉

Fixes #2839
Fixes #3115
Fixes #3487
Closes #3242
Fixes #3765
Fixes #3776

@JoeKar
Copy link
Collaborator Author

JoeKar commented Jan 30, 2024

By default Vim stops at a line length of 3k, but micro now doesn't (care):
grafik

@dustdfg
Copy link
Contributor

dustdfg commented Jan 30, 2024

  1. Can confirm it really solves Micro does not respnoe when edit specific file #3115
    1.1.While I tried to to test if it solves the issue I noticed nothing not desirable
  2. Replaced my current executable with executable of PR (+ some other PR which I believe won't influence this PR) so will say if find regressions

There is a problem maybe related maybe not. Create a file with 1MB of characters on one line and micro will freeze or slow but will work. It will start to work normally when there is no any characters from that big line is displayed on the screen

@JoeKar
Copy link
Collaborator Author

JoeKar commented Jan 31, 2024

Create a file with 1MB of characters on one line and micro will freeze or slow but will work.

This is caused by https://github.com/zyedidia/micro/blob/master/internal/util/unicode.go#L69 resp. utf8.DecodeRune which takes the most of the time. The behavior was more or less the same before.

@dustdfg
Copy link
Contributor

dustdfg commented Feb 1, 2024

Create a file with 1MB of characters on one line and micro will freeze or slow but will work.

This is caused by https://github.com/zyedidia/micro/blob/master/internal/util/unicode.go#L69 resp. utf8.DecodeRune which takes the most of the time. The behavior was more or less the same before.

Yeah but I am almost sure not fully. Why? I tried one time assume that my file will be ascii only so no unicode and I deleted the length checking (https://github.com/dustdfg/micro/tree/unicode_length_unchecked) and performance became better but not much

@JoeKar JoeKar marked this pull request as draft February 1, 2024 20:40
@JoeKar
Copy link
Collaborator Author

JoeKar commented Feb 1, 2024

Yeah but I am almost sure not fully. Why? I tried one time assume that my file will be ascii only so no unicode and I deleted the length checking (https://github.com/dustdfg/micro/tree/unicode_length_unchecked) and performance became better but not much

Yes, you're right. Due to the randomness it finds a lot of "regions" within this long line and starts slicing them one after an other. The problem is, that this slicing process is based on the decoding of the characters too, to identify the correct position of the code points within the byte slice. Currently this isn't that optimized so far, but I've no cool idea how this can be done better.

@JoeKar JoeKar marked this pull request as ready for review February 1, 2024 21:31
@JoeKar JoeKar force-pushed the feature/perf-highlighting branch from a988899 to e9effe8 Compare February 1, 2024 22:47
@dustdfg
Copy link
Contributor

dustdfg commented Feb 2, 2024

Yeah but I am almost sure not fully. Why? I tried one time assume that my file will be ascii only so no unicode and I deleted the length checking (https://github.com/dustdfg/micro/tree/unicode_length_unchecked) and performance became better but not much

Yes, you're right. Due to the randomness it finds a lot of "regions" within this long line and starts slicing them one after an other. The problem is, that this slicing process is based on the decoding of the characters too, to identify the correct position of the code points within the byte slice. Currently this isn't that optimized so far, but I've no cool idea how this can be done better.

Randomness?

@JoeKar
Copy link
Collaborator Author

JoeKar commented Feb 2, 2024

Create a file with 1MB of characters on one line and micro will freeze or slow but will work.

Didn't you use something like the following to create the file?

cat /dev/urandom | tr -dc 'A-Za-z0-9!"#$%&'''()*+,-./:;<=>?@[]^_`{|}~' | head -c 1M > test.sh

That was what I used and due to that I will receive some of "" & '' regions.

@dustdfg
Copy link
Contributor

dustdfg commented Feb 2, 2024

Create a file with 1MB of characters on one line and micro will freeze or slow but will work.

Didn't you use something like the following to create the file?

cat /dev/urandom | tr -dc 'A-Za-z0-9!"#$%&'''()*+,-./:;<=>?@[]^_`{|}~' | head -c 1M > test.sh

That was what I used and due to that I will receive some of "" & '' regions.

Yeah but I used base64 which doesn't include (as I know) any quotes characters + sed (to delete the \n because micro couldn't do it)

@dmaluka
Copy link
Collaborator

dmaluka commented Feb 4, 2024

I believe the problem with very long lines is that micro stores line data in memory simply as an array of raw bytes, not an array of unicode runes. I.e. it doesn't decode the line into runes beforehand. So whenever micro needs to know anything about a visual representation of any part of a line, it decodes it on the fly. (So for instance, if this is a very long line and we are closer to the end of it, micro has to decode almost the entire line, and it has to do that every time when redrawing the screen and so on. So things become extremely slow.)

Heck, micro doesn't even know how many characters are there in a line. It has to call util.CharacterCount() every time to find it out.

If we change it to decode and store lines as slices of runes beforehand, it should solve these problems, and generally improve performance. I really don't get why it wasn't done this way from the beginning.

@JoeKar
Copy link
Collaborator Author

JoeKar commented Feb 4, 2024

Yep, that's an good idea to solve this here as well. Maybe I'll take care of this in the next few days. If someone would like to improve this earlier then give it a go and we will merge it afterwards into this PR. 😉

@dustdfg
Copy link
Contributor

dustdfg commented Feb 4, 2024

If we change it to decode and store lines as slices of runes beforehand, it should solve these problems, and generally improve performance. I really don't get why it wasn't done this way from the beginning.

I see one possible reason: Rune is int32. It would be like using UTF32 for all characters even if our document contains only ASCII

@dmaluka
Copy link
Collaborator

dmaluka commented Feb 4, 2024

Will be great if you take care of it. But why do it in this PR? It is not really a highlighter issue. It is an issue of efficient representation of a line in the basic data structures (so it is more fundamental than the highlighter issues, and affects us even when syntax highlighting is not used at all).

Initial rough idea of where we might start:

--- a/internal/buffer/line_array.go
+++ b/internal/buffer/line_array.go
@@ -41,10 +41,16 @@ type searchState struct {
 	done       bool
 }
 
+type Character struct {
+	r     rune
+	combc []rune
+}
+
 // A Line contains the data in bytes as well as a highlight state, match
 // and a flag for whether the highlighting needs to be updated
 type Line struct {
 	data []byte
+	char []Character
 
 	state       highlight.State
 	match       highlight.LineMatch

@dmaluka
Copy link
Collaborator

dmaluka commented Feb 4, 2024

I see one possible reason: Rune is int32. It would be like using UTF32 for all characters even if our document contains only ASCII

Yes, it costs memory. Isn't it worth it?

@dustdfg
Copy link
Contributor

dustdfg commented Feb 5, 2024

I see one possible reason: Rune is int32. It would be like using UTF32 for all characters even if our document contains only ASCII

Yes, it costs memory. Isn't it worth it?

Idk. I can only say if I would a developer that writes a new app I would prefer to make something on bytes because they are cheaper (in size) just because it is the most obvious solution.

If we have 1MB file we would store 4MB in memory it isn't so bad for most hardware but I am not sure I would want it. Just as a user I would prefer to have different modes which isn't so easy for developer...

@dustdfg
Copy link
Contributor

dustdfg commented Feb 5, 2024

Will be great if you take care of it. But why do it in this PR? It is not really a highlighter issue. It is an issue of efficient representation of a line in the basic data structures (so it is more fundamental than the highlighter issues, and affects us even when syntax highlighting is not used at all).

Yeah it is a separate issue

Initial rough idea of where we might start:

--- a/internal/buffer/line_array.go
+++ b/internal/buffer/line_array.go
@@ -41,10 +41,16 @@ type searchState struct {
 	done       bool
 }
 
+type Character struct {
+	r     rune
+	combc []rune
+}
+
 // A Line contains the data in bytes as well as a highlight state, match
 // and a flag for whether the highlighting needs to be updated
 type Line struct {
 	data []byte
+	char []Character
 
 	state       highlight.State
 	match       highlight.LineMatch

Storing data in bytes and storing the same data in the ints? I can understand replacement but addition? Why?

@dmaluka
Copy link
Collaborator

dmaluka commented Feb 5, 2024

Storing data in bytes and storing the same data in the ints? I can understand replacement but addition? Why?

As I said, this is just an initial rough idea. I hope we won't need to keep both representations. OTOH, for example, if we need to keep LineBytes() for compatibility with plugins... (Some of my plugins do use it, I can change them if needed, but I'm not sure about other peoples plugins...)

@JoeKar
Copy link
Collaborator Author

JoeKar commented Feb 5, 2024

But why do it in this PR? It is not really a highlighter issue.

Yeah it is a separate issue

I give both of you right, that the non-decoded bytes are and thus their repetitive decoding is a common performance degradation independent from the highlighting but the feature suffering the most from it is the highlighting...at least from end user perspective. The impact and result is easier to "feel"/measure with the syntax highlighting and due to the common usage of byte streams this interface this tightly coupled into the highlighting process. A rebase is then most likely. 😉

Anway, I will find a solution for that lazy developer reason. 😄

@dmaluka
Copy link
Collaborator

dmaluka commented Feb 5, 2024

but the feature suffering the most from it is the highlighting...at least from end user perspective.

I don't think so. Try it: create a file long.txt with a long line containing 1 million characters, open it (the filetype will be unknown => no highlighting), move to the end of this long line and, for example, just try moving the cursor around. It's gonna be very slow. And if you also, for example, enable softwrap, it will be very slow even at the beginning of the long line.

Just as an example, how many times do we call RuneUnder() every time we refresh the display in displayBuffer()?

due to the common usage of byte streams this interface this tightly coupled into the highlighting process.

I don't think the highlighting is special here. Replacing byte streams with rune streams, although conceptually simple, is gonna be a huge change, reworking so many parts of code throughout micro (look at all the places where LineBytes() is used, for example). Changes in the highlighter would be just a small part of it, I think.

A rebase is then most likely.

Nothing's wrong with that.

@JoeKar
Copy link
Collaborator Author

JoeKar commented Feb 6, 2024

Ok, you convinced me once again.
I started yesterday in the evening with a small rework (independent from the highlighting):
https://github.com/JoeKar/micro/tree/feature/perf-rune-lines

The branch is highly floating and the result (line handling) far away from a productive state. The start is done and now needs a lot of refactoring, fixing and adaptations at a lot of different locations.

@dustdfg
Copy link
Contributor

dustdfg commented Feb 9, 2024

  1. Can confirm it really solves Micro does not respnoe when edit specific file #3115
    1.1.While I tried to to test if it solves the issue I noticed nothing not desirable

    1. Replaced my current executable with executable of PR (+ some other PR which I believe won't influence this PR) so will say if find regressions

There is a problem maybe related maybe not. Create a file with 1MB of characters on one line and micro will freeze or slow but will work. It will start to work normally when there is no any characters from that big line is displayed on the screen

I still use it and didn't encounter any problems so I think @dmaluka is right and highlight should be separate pull request (and issue?)

@niten94
Copy link
Contributor

niten94 commented Feb 11, 2024

I was looking at /usr/include/unistd.h in amd64 version of libc6-dev package in Debian 11 but there were multi-line comments with text not highlighted as text in a comment like this:
image

I was looking at this file when testing a bit: c-comment.c.txt
image

I think only the line where a multi-line comment is started is highlighted as text in a comment if there is text in it highlighted using regions when not in a comment. The other text in the line is also not highlighted as text in a comment if the region would end in it when not in a comment.

The lines between the first and last line in a character literal are not highlighted as text with an error. The other text in the line where a character literal ends is highlighted as text in a literal if the literal ends in another line.

There are similar bugs but I do not know if they are actually the same so I have not written about them.

@dmaluka
Copy link
Collaborator

dmaluka commented Feb 11, 2024

@niten94 I can also see this bug (and don't see it without this PR).

@JoeKar
Copy link
Collaborator Author

JoeKar commented Feb 11, 2024

[...] so I think @dmaluka is right and highlight should be separate pull request (and issue?)

Yes, this one wouldn't be mixed up with the other idea/improvement optimizing and storing the line data.

@niten94 I can also see this bug (and don't see it without this PR).

P.S. This is not the most important thing at the moment. I'd suggest to first address the regression reported by @niten94 in #3127 (comment) because maybe it means that the entire approach of this PR is wrong.

Yes, it's a regression which will be fixed in the next few days. Sorry for the inconveniences. AFAIK it sounds again like a logic issue within the nested region detection, which now uses all indices found in a line.
But I thank you already for the testing, because this is really welcome here.

To make the API more intuitive, I'd suggest to remove statesOnly from Highlight() (but not from the internal highlight() function) and restore the ReHighlightStates() function which would call highlight() with statesOnly=true.

Side note: it seems we have redundant steps in this loop, which we can avoid by changing i++ to i = l + 1.

I will consider both.

@JoeKar JoeKar marked this pull request as draft February 11, 2024 19:48
@JoeKar JoeKar force-pushed the feature/perf-highlighting branch from e9effe8 to 103ed6d Compare February 14, 2024 17:28
@JoeKar JoeKar force-pushed the feature/perf-highlighting branch from 4ab825b to fb2ed1b Compare October 20, 2024 14:49
@JoeKar JoeKar requested a review from dmaluka October 23, 2024 18:24
@JoeKar JoeKar force-pushed the feature/perf-highlighting branch from fb2ed1b to 89216ed Compare December 17, 2024 19:41
regionStart := 0
regionEnd := 0
for _, r := range regions {
if !nestedRegion && curRegion != nil && (curRegion.group != r.group || curRegion.start != r.start) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this about?

And what exactly is the meaning of nestedRegion (and what is it for)? Does it mean "we are inside a region that is inside another region"? If so, why do we need to pass it as an extra argument to highlightRegions() instead of just using currentRegion != nil && currentRegion->parent != nil, and more importantly, why do we even care whether this region is inside another region or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nestedRegion is there to tell the logic that the current region is still "open" and child patterns and regions are to be scanned. But I agree, that the name is somehow misleading, since it controls this 2-stage approach...

  1. Try to find the regions end in the line.
  2. Try to find child patterns/regions till the found end or end of the line.

So maybe processNesting, processChilds or something different is better?

Currently nestedRegion is not the same as currentRegion != nil and not the same as currentRegion != nil && currentRegion->parent != nil.

Copy link
Collaborator

@dmaluka dmaluka Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current region is still "open" and child patterns and regions are to be scanned.

What does this mean? Aren't child patterns and regions always to be scanned? I.e. when the current region is "closed", it simply means that its parent region becomes the current region? And the only case when it doesn't happen is when there is no parent region, i.e. when curRegion->parent == nil (or even just curRegion == nil)? So how is nestedRegion different from curRegion->parent != nil (or even just curRegion != nil)?

I see that in all cases when highlightRegions() is called with with nestedRegion=true, it is called with regions set to r.rules.regions (i.e. to the children of its curRegion), and in all cases when highlightRegions() is called with with nestedRegion=false, it is called with regions set to h.Def.rules.regions i.e. to the top-level regions (so it can only be correct if it is called with curRegion=nil?). So it seems we indeed have some suspicious redundancy here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you, that this part needs some more rework, to remove the additional and redundant parameter. At the end it is then hopefully easier to follow.

highlights[start+loc[1]] = 0
h.highlightEmptyRegion(highlights, start+loc[1], lineNum, util.SliceEnd(line, loc[1]))
return highlights
if curRegion != nil && curRegion.group == r.group && curRegion.start == r.start {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this if really be inside the loop? It seems like this makes the outcome dependent on the order of regions in regions, whereas this order is, from our perspective, completely random, we shouldn't rely on it in any way?

...IIUC, the high-level problems we need to solve here in highlightRegions() are:

  1. Find the candidates for sub-regions in the current region. (IIUC, basically the above nested 2-level loop addresses it.)
  2. Find the candidates for the end of the current region. (IIUC, basically this if addresses it.)

We need to correctly solve both these problems together, and it is not obvious to me how to do that, since problem 1 depends on problem 2.

Could you provide a high-level summary of how your code is solving both these problems together?

(Right now I only see that samePattern somehow connects them together, and I don't understand what the hell is this samePattern doing.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this if really be inside the loop? It seems like this makes the outcome dependent on the order of regions in regions, whereas this order is, from our perspective, completely random, we shouldn't rely on it in any way?

Besides the fact that this line is slightly changed with...
e33ec52#diff-16d0164b7fcc4350a9a7a5827b930e8de29487a35a3afa8d9e165b8e2af0f13aL213-R213
...does it really depend on the order? From my perspective at least not within the same level of region rules. We've a particular region still "open" and search for its end. We find a region, which fits to the "open" one and check for further details. The only ambiguity which could apply is, that a region type has further rules with the "same" region type, but still then curRegion is not equal to r, since they store pointer and they will differ in this case and so there is no ambiguity.

Could you provide a high-level summary of how your code is solving both these problems together?

With #3127 (comment) I started giving some insights.
Both problems are currently solved, more or less as before, by calling the same function in itself, while nestedRegion controls the operation mode (if it currently checks for sub-regions/-patterns or not). Additionally regions will then host sub-rules of its parent, if available.

  1. Find the candidates for the end of the current region. (IIUC, basically this if addresses it.)

Not this if alone, but this branch too...
https://github.com/zyedidia/micro/pull/3127/files#diff-16d0164b7fcc4350a9a7a5827b930e8de29487a35a3afa8d9e165b8e2af0f13aR279
...and inside we use h.highlightRegions(..., true) again to search for possible sub-rules.

(Right now I only see that samePattern somehow connects them together, and I don't understand what the hell is this samePattern doing.)

Locally I already removed the content inside this branch...
https://github.com/zyedidia/micro/pull/3127/files#diff-16d0164b7fcc4350a9a7a5827b930e8de29487a35a3afa8d9e165b8e2af0f13aR261-R277
...and simplified the definition of samePattern to samePattern := r.start.String() == r.end.String(), because this is what samePattern shall represent (start & end are defined by the same string, e.g. " or ' etc).

JoeKar and others added 17 commits March 25, 2025 20:21
We break away from the assumption that the highlighter will be reused in
different projects.
The most expensive path in the highlighting process is the regex parsing
step which was invoked for every line twice due to the split approach.
Since this can be done in one step the highlighting effort can be reduced
by around 50 percent!
The old approach was to slide over the line by (empty) region over region one
after another. Step down into a possible available nested part and do the same.

The new approach iterates over all available region definitions and shall find
all occurrences of the same region at the same time (`findAllIndex()`) and not
just the first (`findIndex()`). This will allow marking a region already in the
moment it isn't the first region at the line. After an region has been
identified all nested parts are considered the same way.

Simple example (no nesting):

1. region start=" end="
2. region start=' end='
3. region start=< end=>

The "first", "second" and "third" are highlighted in one round.
This 'highlights after ""' while <highlights last> and "highlights first".
This is just my interpretation of `limitGroup` as it might be intended
a long time ago, but this interpretation could be wrong,
since it is not really documented how it should be.
This is necessary in the moment sibling regions overlap each other.
@JoeKar JoeKar force-pushed the feature/perf-highlighting branch from 89216ed to db7581c Compare March 25, 2025 23:11
@JoeKar JoeKar marked this pull request as draft April 26, 2025 18:44
@JoeKar
Copy link
Collaborator Author

JoeKar commented Apr 26, 2025

Convert to draft again, since there are still scenarios not covered by the current proposal.
@dmaluka rightly complained that the logic is difficult to understand and not really comprehensible.
I need to do a larger rework to simplify the logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

5 participants